refactor: decouple tooltip from XY chart #553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markov00
force-pushed
the
2020_02_13-decouple_tooltip
branch
from
0f9d96a
to
4344c73
Compare
BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simple object with two common values: specId and key. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`
The commit change the naming for few variables in the legend item object, removing some unnecessary code BREAKING CHANGE: The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra`
Codecov Report
@@ Coverage Diff @@
## master #553 +/- ##
=======================================
Coverage 71.59% 71.59%
=======================================
Files 212 212
Lines 6137 6137
Branches 1138 1138
=======================================
Hits 4394 4394
Misses 1724 1724
Partials 19 19Continue to review full report at Codecov.
|
The isXValue is no longer required inside the TooltipValue object because we are already defining the header and the values inside two distinct objects
I've renamed the yAccessor field in favour of a more general valueAccessor. I've also added the seriesIdentifier to each tooltipValue
nickofthyme
approved these changes
Feb 19, 2020
| /** | ||
| * Show an extra parameter on each legend item defined by the chart type | ||
| */ | ||
| showLegendExtra: boolean; |
There was a problem hiding this comment.
can we rename this to hideLegendExtra so the default is false? Then it would just be.
<Settings hideLegendExtra />There was a problem hiding this comment.
I think we should have the extra only visible if the user explicitly enable them (#246)
There was a problem hiding this comment.
Ok in that case shouldn’t showLegendExtra default be false?
| @@ -1028,21 +1034,27 @@ describe.skip('Chart Store', () => { | |||
| store.annotationDimensions.set(rectAnnotationSpec.id, annotationDimensions); | |||
|
|
|||
| const highlightedTooltipValue: TooltipValue = { | |||
| name: 'foo', | |||
| label: 'foo', | |||
There was a problem hiding this comment.
Can we pick name or label to be consistent? I am having trouble choosing between the two. If we are going to have a name prop where this value is derived from, this should be name not label right?
There was a problem hiding this comment.
Do you mean the name prop of the Series component?
There was a problem hiding this comment.
Correct, this values is determined from the customSeriesLabel, soon to be name prop.
There was a problem hiding this comment.
ok let me switch to name then
There was a problem hiding this comment.
from our yesterday call, we will keep the label naming to make the tooltip a bit more abstract from the series. The Label should refer to the descriptive part of the value
change the default value of `showLegendExtra` to `false` BREAKING CHANGE: Previously `showLegendExtra` was true by default always showing the legend extra value. Now it becomes always hidden by default close elastic#246
markov00
changed the title
markov00
added a commit
that referenced
this pull request
Feb 21, 2020
This commit will decouple the tooltip component from the XY chart to allow Partition and other chart type an ease use. BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default. close #246
markov00
added a commit
that referenced
this pull request
Mar 2, 2020
This commit will decouple the tooltip component from the XY chart to allow Partition and other chart type an ease use. BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default. close #246
markov00
pushed a commit
that referenced
this pull request
Mar 17, 2020
# [18.0.0](v17.1.1...v18.0.0) (2020-03-17) ### Code Refactoring * clean up TS types ([#554](#554)) ([22f7635](22f7635)), closes [#547](#547) [#547](#547) * decouple tooltip from XY chart ([#553](#553)) ([e70792e](e70792e)), closes [#246](#246) ### Features * cleaner color API on SeriesSpec ([#571](#571)) ([f769f7c](f769f7c)) * **legend:** allow color picker component render prop ([#545](#545)) ([90f4b95](90f4b95)) * **partition:** add element click, over and out events ([#578](#578)) ([103df02](103df02)) * **partition:** add tooltip ([#544](#544)) ([6bf9a69](6bf9a69)), closes [#246](#246) * percentage display in partitioning charts ([#558](#558)) ([d6aa8d7](d6aa8d7)) * specify series name with a function on SeriesSpec ([#539](#539)) ([358455a](358455a)) * xAccessor can be a function accessor ([#574](#574)) ([bcc3d63](bcc3d63)) ### BREAKING CHANGES * The `getSpecId`, `getGroupId`, `getAxisId` and `getAnnotationId` are no longer available. Use a simple `string` instead. * `customSeriesColors` prop on `SeriesSpec` is now `color`. The `CustomSeriesColors` type is replaced with `SeriesColorAccessor`. * Remove `customSubSeriesName` prop on series specs in favor of cleaner api using just the `name` prop on `SeriesSpec`. The types `SeriesStringPredicate`, `SubSeriesStringPredicate` have been removed. * the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.
2 tasks
AMoo-Miki
pushed a commit
to AMoo-Miki/OpenSearch-Dashboards
that referenced
this pull request
Feb 10, 2022
# [18.0.0](elastic/elastic-charts@v17.1.1...v18.0.0) (2020-03-17) ### Code Refactoring * clean up TS types ([opensearch-project#554](elastic/elastic-charts#554)) ([6ce56fa](elastic/elastic-charts@6ce56fa)), closes [opensearch-project#547](elastic/elastic-charts#547) [opensearch-project#547](elastic/elastic-charts#547) * decouple tooltip from XY chart ([opensearch-project#553](elastic/elastic-charts#553)) ([cb02cd0](elastic/elastic-charts@cb02cd0)), closes [opensearch-project#246](elastic/elastic-charts#246) ### Features * cleaner color API on SeriesSpec ([opensearch-project#571](elastic/elastic-charts#571)) ([6a78c4e](elastic/elastic-charts@6a78c4e)) * **legend:** allow color picker component render prop ([opensearch-project#545](elastic/elastic-charts#545)) ([22ef1e6](elastic/elastic-charts@22ef1e6)) * **partition:** add element click, over and out events ([opensearch-project#578](elastic/elastic-charts#578)) ([4189573](elastic/elastic-charts@4189573)) * **partition:** add tooltip ([opensearch-project#544](elastic/elastic-charts#544)) ([0cffed4](elastic/elastic-charts@0cffed4)), closes [opensearch-project#246](elastic/elastic-charts#246) * percentage display in partitioning charts ([opensearch-project#558](elastic/elastic-charts#558)) ([993a448](elastic/elastic-charts@993a448)) * specify series name with a function on SeriesSpec ([opensearch-project#539](elastic/elastic-charts#539)) ([fc6430b](elastic/elastic-charts@fc6430b)) * xAccessor can be a function accessor ([opensearch-project#574](elastic/elastic-charts#574)) ([f702e2c](elastic/elastic-charts@f702e2c)) ### BREAKING CHANGES * The `getSpecId`, `getGroupId`, `getAxisId` and `getAnnotationId` are no longer available. Use a simple `string` instead. * `customSeriesColors` prop on `SeriesSpec` is now `color`. The `CustomSeriesColors` type is replaced with `SeriesColorAccessor`. * Remove `customSubSeriesName` prop on series specs in favor of cleaner api using just the `name` prop on `SeriesSpec`. The types `SeriesStringPredicate`, `SubSeriesStringPredicate` have been removed. * the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR will decouple the tooltip from the XY chart to allow Partition and other chart type to use it.
BREAKING CHANGES
the
SeriesIdentifiertype is generalised into a simple object with two common values: specId and key. A specialisedXYChartSeriesIdentifierextends now the baseSeriesIdentifierThe
SettingsSpecpropshowLegendDisplayValueis renamed toshowLegendExtra.Previously
showLegendDisplayValuewas true by default always showing the legend extravalue. Now it becomes always hidden by default
close #246 this PR will hide the value by default, we will iterate over this to make this parameter customisable #561
Note for reviewers
It's easier to follow the changes one commit a time, as they are self contained.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.src/index.ts(and stories only import from../srcexcept for test data & storybook)